Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support cloudflare options with setItem #255

Closed
wants to merge 10 commits into from

Conversation

Hebilicious
Copy link
Member

@Hebilicious Hebilicious commented Jun 30, 2023

πŸ”— Linked issue

Resolves #32

First part of #236

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This PR allows the ability to pass options to the setItem method while using the cloudflare driver :

await storage.setItem("key", "value", 
{ 
  expiration: 1578435000, 
  expiration_ttl: 300, // http driver
  expirationTtl: 300, // binding driver
  base64: false,
  metadata: { someMetadataKey: "someMetadataValue" }
});
  • implements the cloudflare options such as metadata, expiring keys, etc.
    • Forward the 3rd argument (options) to the cloudflare-kv-bindingand cloudflare-kv-http drivers setItem method.
    • Changes the put implementation of the cloudflare-kv-http to use the bulk API (to support all options).

Options "API design"

For now I'm leverage the 3rd argument of the setItem method, but I was thinking that we could require users to pass them nested under a key ({ cloudflare: {} }), or use a 4th argument, in-case we want to keep the 3rd argument open for unstorage specific options... Thoughts ?

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@nuxt-studio
Copy link

nuxt-studio bot commented Jun 30, 2023

βœ… Live Preview ready!

Name Edit Preview Latest Commit
unstorage Edit on Studio β†—οΈŽ View Live Preview ee82649

@Hebilicious Hebilicious changed the title feat: supports cloudflare options feat: support cloudflare options Jun 30, 2023
@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Merging #255 (42112e3) into main (e41a1df) will increase coverage by 0.56%.
Report is 1 commits behind head on main.
The diff coverage is 93.04%.

@@            Coverage Diff             @@
##             main     #255      +/-   ##
==========================================
+ Coverage   77.96%   78.52%   +0.56%     
==========================================
  Files          29       29              
  Lines        3417     3483      +66     
  Branches      521      522       +1     
==========================================
+ Hits         2664     2735      +71     
+ Misses        752      747       -5     
  Partials        1        1              
Files Changed Coverage Ξ”
src/drivers/http.ts 85.56% <85.71%> (ΓΈ)
src/drivers/cloudflare-kv-binding.ts 84.46% <89.47%> (+0.74%) ⬆️
src/drivers/cloudflare-kv-http.ts 89.57% <93.33%> (+5.10%) ⬆️
src/drivers/cloudflare-r2-binding.ts 77.57% <100.00%> (ΓΈ)
src/types.ts 100.00% <100.00%> (ΓΈ)

πŸ“’ Have feedback on the report? Share it here.

@Hebilicious Hebilicious requested review from pi0 and farnabaz and removed request for pi0 June 30, 2023 15:08
@Hebilicious Hebilicious added the enhancement New feature or request label Jun 30, 2023
@Hebilicious Hebilicious requested review from pi0 and removed request for farnabaz June 30, 2023 15:12
@Hebilicious Hebilicious changed the title feat: support cloudflare options feat: support cloudflare options with setItem Jun 30, 2023
@Hebilicious Hebilicious assigned pi0 and unassigned pi0 and Hebilicious Jun 30, 2023
@Hebilicious Hebilicious added the ready label Jul 1, 2023 — with Volta.net
@Hebilicious Hebilicious added pending and removed ready labels Jul 4, 2023
@Hebilicious
Copy link
Member Author

Refactor with namespace

@Hebilicious
Copy link
Member Author

Hebilicious commented Jul 8, 2023

Added a generic ttl parameters, as well as a default ttl configurable in the options.
Also added namespaced options under the kebabcased driver name : cloudflareKvBinding and cloudflareKvHttp.
Since they are different we can't use the same key.

I noticed that you didn't nest the cloudflare r2 options under a key, so for now we need to be more explicit with any. This should be fixed in another PR.

There's also a bunch of new bugs that this PR fixes in r2 and http, as we were using any before.

@pi0 I think we need multiple PRs:

  1. Refactor TransactionOptions into 2 new types that are more explicit, SetItemOptions and GetItemOptions, and have the generic ttl?: number, and fix the type bugs without changing behaviours.
  2. Refactor R2 with a nested key (id say make the breaking change as its only 4d old, would give full type safety, and we discussed this already)
  3. Add the SetItemOptions for cfkvb and cfkvhttp according to this PR.

@pi0
Copy link
Member

pi0 commented Jul 17, 2023

Refactor TransactionOptions into 2 new types that are more explicit, SetItemOptions and GetItemOptions, and have the generic ttl?: number, and fix the type bugs without changing behaviours.

As i mentioned in other PR, it would make sense to support one kind of options interface (even if some option keys are not used by read operation). Particulary for ttl that should be also supported for passive key expiration for drivers not supporting ttl.

Refactor R2 with a nested key (id say make the breaking change as its only 4d old, would give full type safety, and we discussed this already)

πŸ‘πŸΌ

Copy link
Member Author

@pi0 Should we still introduce ttl in a separated PR #271 (TransactionOptions) ? It causes some minor issues with some existing presets, hence why its cleaner separately.
But if you prefer just doing everything through this one I'm fine either way.

@pi0
Copy link
Member

pi0 commented Jul 17, 2023

If it is only for type, surely we can add ttl? type to generic TransactionOptions in this PR πŸ‘πŸΌ

@Hebilicious
Copy link
Member Author

@pi0 I would be happy to revisit splitting TransactionOptions once it's actually needed. For now I've implemented everything in this PR according to your requirements.
http and r2 needs to be slightly tweaked because we are no longer using any for TransactionOptions. This doesn't affect the runtime.

@pi0 pi0 added pending and removed ready labels Sep 8, 2023
@pi0
Copy link
Member

pi0 commented Sep 8, 2023

Thanks for the updates. As an update, i still need to double check all changes there are too many changes some are unrelated to the feature we want to add.

@Hebilicious
Copy link
Member Author

Hebilicious commented Sep 8, 2023

Thanks for the updates. As an update, i still need to double check all changes there are too many changes some are unrelated to the feature we want to add.

@pi0 I had to change these things :

@pi0
Copy link
Member

pi0 commented May 1, 2024

PR is stalled and with too many changes, have to rework this.

(but again really appreciate your previous help on this ❀️ )

@pi0 pi0 closed this May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Cloudflare KV native meta
2 participants